Repository navigation
fix(api): harden shielded proof generation and note scans - #7096
Federico2014 wants to merge 4 commits into
Conversation
|
[NIT / Hardening] Validate the diversifier in Both existing builders already reject invalid diversifiers before invoking the output-proof wrapper:
The encryption path rejects invalid diversifiers through Note.encrypt() and NoteEncryption.fromDiversifier(), which returns an empty result when However, JLibrustzcash.librustzcashSaplingOutputProof() forwards parameters directly to native code without independently validating the diversifier. Calling Consider adding parameter validation and an explicit diversifier check at the wrapper boundary, along with a regression test confirming that an invalid diversifier is rejected before the native output-proof call. This is a defense-in-depth improvement for future direct callers; the two existing builder paths are already protected. |
| params.valid(); | ||
| INSTANCE.librustzcashZip32XskDerive(params.getData(), params.getSize(), params.getM_bytes()); | ||
| } | ||
|
|
||
| public static boolean librustzcashZip32XfvkAddress(Zip32XfvkAddressParams params) { | ||
| public static boolean librustzcashZip32XfvkAddress(Zip32XfvkAddressParams params) | ||
| throws ZksnarkException { | ||
| params.valid(); |
There was a problem hiding this comment.
[NIT / SDK & Native Hardening] Handle invalid serialized ZIP32 keys without native panics.
The new params.valid() checks validate buffer lengths but do not validate the serialized extended-key contents:
- librustzcashZip32XskDerive():
Zip32XskDeriveParams.valid()checks the 169-byte buffer lengths, but native decoding ofxsk_parentstill uses ExtendedSpendingKey::read(...).expect(...). - librustzcashZip32XfvkAddress():
Zip32XfvkAddressParams.valid()checks buffer lengths, but native decoding ofxfvkstill uses ExtendedFullViewingKey::read(...).expect(...).
Consequently, correctly sized buffers containing invalid serialized keys can still trigger a native panic during decoding.
I found no production callers of these wrappers in the current java-tron tree, so this can be tracked as a follow-up rather than blocking this PR.
Before accepting untrusted extended keys through these interfaces, please replace the native expect() calls with controlled error handling and propagate failures safely through JNI and the SDK. The derive API currently returns void, so its failure-reporting mechanism also needs to be addressed.
Please add regression tests using correctly sized but invalid serialized XSK/XFVK values, verifying that decoding failures are handled without native panics.
The absence of production callers in java-tron does not establish whether other SDK consumers expose these functions to untrusted inputs.
There was a problem hiding this comment.
@3for Agreed. The length checks do not validate serialized key contents, so this remains incomplete, as noted in the PR description.
I'll address it in a separate SDK/native follow-up: replace both decoding expect() calls with controlled error handling, propagate failures through JNI and Java—including an explicit failure contract for XSK derivation—and add regression tests using correctly sized but invalid XSK/XFVK inputs. Other SDK consumers also need to be considered, even though these wrappers currently have no production callers in java-tron.
| result); | ||
| if (!JLibrustzcash.librustzcashComputeNf(computeNfParams)) { | ||
| return null; | ||
| throw new ZksnarkException("compute nullifier error"); |
There was a problem hiding this comment.
[SHOULD] Native nullifier failures still cannot reach this check.
JLibrustzcash.librustzcashComputeNf() calls the SDK and unconditionally returns true. The SDK Java method returns void, and the JNI bridge discards Rust’s boolean return value.
Rust can return false before writing the output buffer. The Java-side ComputeNfParams.valid() checks lengths and non-negative values, but does not cover all native semantic checks. Consequently, createShieldNullifier() can still return the untouched output buffer as a successful result.
The new null check in isSpend() has the same limitation: Note.nullifier() returns null only when that wrapper returns false.
Please preserve an explicit success/failure status through Rust → JNI → SDK Java → JLibrustzcash, or provide an equivalent reliable error contract. Add regression coverage across that boundary: the current failure tests mock JLibrustzcash itself, so they exercise the Wallet branches without detecting the status loss. An all-zero output check should not substitute for the success status.
There was a problem hiding this comment.
@3for Agreed. The current checks do not complete the fix: JNI discards the native return value, the SDK method returns void, and librustzcashComputeNf() always returns true. The mocked tests only verify the Wallet failure branches.
As noted in the PR description, this remains incomplete until the SDK/JNI/native changes are published and the dependency is upgraded. I'll wire librustzcashSaplingComputeNfChecked through the Java wrapper and add regression coverage exercising a real native failure through JNI and the SDK, verifying that it cannot produce a successful nullifier result. An all-zero output check will not substitute for an explicit failure status.
|
[NIT / Hardening] Stop ZK-SNARK initialization when parameter file copying fails. In ZksnarkInitService.getParamsFile(), an As a result, initialization can proceed with a missing or incomplete parameter file. The native implementation uses Please stop initialization when copying fails, clean up any partially written file, and return the destination path only after a successful copy. As parameter initialization runs during normal node startup, including when shielded transaction APIs are disabled. Please also add a regression test verifying that a copy failure prevents the native initialization call. This can be addressed entirely within java-tron as a non-blocking follow-up. |
|
@3for I've implemented both hardening changes:
The targeted tests, Checkstyle, Math usage scan, and diff checks passed. |
Revalidate output-proof parameters and reject invalid diversifiers before calling native proof generation. Stop ZK-SNARK initialization on parameter copy failures and clean up partial files while preserving the cause. Cover both failure boundaries, consolidate initialization tests in TronErrorTest, and remove redundant shielded test assertions and coverage.
| @@ -67,7 +68,14 @@ private static String getParamsFile(String fileName) { | |||
| } | |||
| FileUtils.copyToFile(in, fileOut); | |||
There was a problem hiding this comment.
[NIT] Could we allocate these parameter files atomically with Files.createTempFile, preferably in a node-private temporary directory, instead of constructing timestamp-based paths? Please preserve the new cleanup and error propagation behavior. The native hash check validates parameter contents, but does not establish ownership of the destination used by the earlier copy operation.
What does this PR do?
Reject both identity encodings of spend authorization keys at the shared Java proof boundary, revalidate mutable parameters, and validate ZIP32 array lengths and seed bounds. Check native spend-signature and binding-signature results before publishing output, and guard Wallet nullifier failure paths.
Skip authenticated notes with unsupported plaintext headers or negative values in both decryption paths, allowing native shielded and TRC20 IVK/OVK scans to continue past malformed notes.
Why are these changes required?
Identity spend keys can terminate the node during native proof generation. Ignored signature failures can expose uninitialized output, while malformed notes can abort an entire scan range. These changes reject failures at shared API boundaries while preserving valid proof construction and scan results.
This PR has been tested by:
ShieldedNativeValidationTest,ShieldedSignatureFailureTest,ShieldedNullifierFailureTest,WalletShieldedNoteScanTest, andSaplingNoteTestafter integration.Follow up
Publish the separate SDK/JNI/Rust fixes, then upgrade the dependency and connect
librustzcashSaplingComputeNfChecked. The current wrapper still returns true, so native nullifier failure propagation and ZIP32 native parser hardening remain incomplete in java-tron.Extra details
No dependency, consensus verification, or public
Note.decodechanges. ZIP32 checks preserve existing empty/prefix-seed behavior and unsigned hardened indices. Validation was on macOS x86_64 / JDK 8; full API-process, ARM, and full-suite validation remain outstanding.